Add source map spec tests #505
Conversation
test/test-spec-tests.js
Outdated
| // Known failures due to intentional implementation choices or due to bugs. | ||
| const skippedTests = [ | ||
| // Versions are explicitly checked a bit loosely. | ||
| "versionNumericString", | ||
| // Stricter sources array checking isn't implemented. | ||
| "sourcesNotStringOrNull", | ||
| "sourcesAndSourcesContentBothNull", | ||
| // Stricter names array checking isn't implemented. | ||
| "namesMissing", | ||
| "namesNotString", | ||
| // This check isn't as strict in this library. | ||
| "invalidMappingNotAString1", | ||
| // A mapping segment with no fields is technically invalid in the spec. | ||
| "invalidMappingSegmentWithZeroFields", | ||
| // These tests fail due to imprecision in the spec about the 32-bit limit. | ||
| "invalidMappingSegmentWithColumnExceeding32Bits", | ||
| "invalidMappingSegmentWithOriginalLineExceeding32Bits", | ||
| "invalidMappingSegmentWithOriginalColumnExceeding32Bits", | ||
| // A large VLQ that should parse, but currently does not. | ||
| "validMappingLargeVLQ", | ||
| // The library currently doesn't check the types of offset lines/columns. | ||
| "indexMapOffsetLineWrongType", | ||
| "indexMapOffsetColumnWrongType", | ||
| // The spec is not totally clear about this case. | ||
| "indexMapInvalidBaseMappings", | ||
| // The spec's definition of overlap can be refined | ||
| "indexMapInvalidOverlap", | ||
| // Not clear if this test makes sense, but spec isn't clear on behavior | ||
| "validMappingNullSources" | ||
| ] |
There was a problem hiding this comment.
Are these skipped tests specific to the mozilla sourcemap implementation?
There was a problem hiding this comment.
Yes, for example the "versionNumericString" test is skipped because this library specifically accepts the case where the version field is a number in a string. Other implementations have different behavior (e.g., the source map validator uses this library but layers more strict checking on top).
There was a problem hiding this comment.
Ok thanks. that makes sense. Fixing these will help align to the spec.
test/test-spec-tests.js
Outdated
| exports[`test from source map spec tests, name: ${testCase.name}`] = | ||
| async function (assert) { | ||
| const json = await readJSON(`./source-map-tests/resources/${testCase.sourceMapFile}`); | ||
| let sourceMapFailed = false; |
There was a problem hiding this comment.
This does not seem to be used.
There was a problem hiding this comment.
Thanks for catching that, I think it was a vestige of an earlier attempt. I'll remove it next time I update the patch.
|
Thanks for this patch. this is looking great. I just had a quick look. I'll be testing it in a little bit |
|
Note: I updated the patch with support for checking generated->original mapping (before it just checked original->generated). Now it depends on PR #507 as well. I also updated it with checking for transitive mapping. Both of these are pretty localized changes. |
test/test-spec-tests.js
Outdated
| map.eachMapping(() => {}); | ||
| map.destroy(); | ||
| } catch (exn) { | ||
| if (testCase.sourceMapIsValid) |
There was a problem hiding this comment.
| if (testCase.sourceMapIsValid) | |
| if (testCase.sourceMapShouldBeValid) |
There was a problem hiding this comment.
This is not related directly to this patch but it just a tiny suggestion while trying the understanding the code.
sourceMapShouldBeValid might make it clearer what the expectation is.
|
Thanks @takikawa. I tested out the patch and it works nicely. I've added a few inline comments. Once the tests have been moved to the main repo we can updated and land this patch. Do you know the timeline for this? Also could we automate the update of the git sub-module by adding the command to the npm task for running tests in the This is a basic inital solution, it can be improved with followups later |
Thanks for the comments! We've now imported the tests into a more official repo (https://github.com/tc39/source-map-tests). I'll work on updating this PR to address your comments and update for the new location soon (hopefully this week). |
This commit adds a submodule for the draft source map spec test repo and adds a new test file that runs each of the test cases. Some test cases that have known failures are skipped, with a comment explaining why.
* Don't do reverse lookup test if source is null * Allow "null" in cases where null is expected
These aren't supported yet because the library doesn't support ignoreList yet and doesn't do any checking for it.
|
I've update the PR and pointed it at the new repo, and addressed your comments (except one, commented above). Happy to address any further feedback. |
Also makes the linter ignore the submodule
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #505 +/- ##
==========================================
+ Coverage 93.90% 94.11% +0.20%
==========================================
Files 13 13
Lines 2988 2991 +3
==========================================
+ Hits 2806 2815 +9
+ Misses 182 176 -6 ☔ View full report in Codecov by Sentry. |
|
@takikawa The patch LGTM. Added a comment to resolve the failure in the jobs. |
Fix the require of the fs promises module
This is a draft PR for issue #504.
The PR adds integration to run the in-development source map specification tests. The tests are accessed via a git submodule, though the current location of the tests is likely not their final official location. The tests are still under development as well.